Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Productionize RMN remote contract #1431

Merged

Conversation

RyanRHall
Copy link
Contributor

@RyanRHall RyanRHall commented Sep 11, 2024

Background

Original PR - there is some discussion here that is possibly still relevant)
Open PR to add cursing - I copied the relevant parts for the RMNRemote.

Note: we are intentionally separating RMNRemote and RMNHome since they have different timelines.

Motivation

Productionize the RMNRemote and include it in integration tests

Copy link
Contributor

github-actions bot commented Sep 11, 2024

LCOV of commit 57da48d during Solidity Foundry #8237

Summary coverage rate:
  lines......: 97.7% (2279 of 2333 lines)
  functions..: 94.6% (420 of 444 functions)
  branches...: 93.6% (542 of 579 branches)

Files changed coverage rate: n/a

Comment on lines 240 to 232
return
s_cursedSubjectsIndexPlusOne[LEGACY_CURSE_SUBJECT] > 0 || s_cursedSubjectsIndexPlusOne[GLOBAL_CURSE_SUBJECT] > 0;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this legacy compatability stuff I am less familiar with. I would appreciate another set of eyes.

@RyanRHall RyanRHall force-pushed the CCIP-3379-productionize-rmn-remote-and-rmn-home-contracts branch from e434b40 to d199d09 Compare September 12, 2024 16:46
@RyanRHall RyanRHall force-pushed the CCIP-3379-productionize-rmn-remote-and-rmn-home-contracts branch from d199d09 to da72d74 Compare September 12, 2024 17:51
commit aae76b37e0397728ebd1a529a265da4f1dec23bc
Author: Ryan Hall <[email protected]>
Date:   Mon Sep 16 12:46:32 2024 -0400

    fix rebase issues

commit a74b43a8f7ccd3d49cb1dc93e3ba69e2e5343d57
Author: Rens Rooimans <[email protected]>
Date:   Mon Sep 16 11:57:15 2024 +0200

    rm VersionedConfig & emit version and config separate

commit db534f9ff05c150633ac7e94362120bd920f1506
Author: Rens Rooimans <[email protected]>
Date:   Mon Sep 16 11:39:04 2024 +0200

    update some tests

commit 3e86f2149229f7634c35224935dcb08b5ccbf945
Author: Rens Rooimans <[email protected]>
Date:   Mon Sep 16 10:57:45 2024 +0200

    use rawVs from report & fix tests

commit fbe12de74e142cff71c47109ee96be27de168cc4
Author: Rens Rooimans <[email protected]>
Date:   Fri Sep 13 12:55:27 2024 +0200

    demo
@RyanRHall RyanRHall force-pushed the CCIP-3379-productionize-rmn-remote-and-rmn-home-contracts branch from 4d52586 to 57da48d Compare September 17, 2024 17:44
@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@RyanRHall RyanRHall enabled auto-merge (squash) September 17, 2024 18:52
@RyanRHall RyanRHall merged commit f95dce6 into ccip-develop Sep 18, 2024
126 checks passed
@RyanRHall RyanRHall deleted the CCIP-3379-productionize-rmn-remote-and-rmn-home-contracts branch September 18, 2024 07:36
RyanRHall added a commit that referenced this pull request Sep 18, 2024
## Motivation

#1431 (comment)

## Solution
Expose the constant as public
RensR pushed a commit to smartcontractkit/chainlink that referenced this pull request Oct 1, 2024
github-merge-queue bot pushed a commit to smartcontractkit/chainlink that referenced this pull request Oct 9, 2024
* Update .gitignore

* assert fees can be set to zero (#1380)

Assert we would have the option to set any of the fee components to zero
if we would want.

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>

* pnpm i && snapshot && forge fmt

* Misc golfs and fixes (#1402)

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>

* Increase max signers (#1405)

Improve tests and error checks

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>

* rename CCIPSendRequested to CCIPMessageSent (#1409)

* Skip report execution on curse (#1408)

The goal of this PR is to remove reverts on lane cursing for DON
execution transactions. If a lane is cursed, reverting will cause the
whole execution transaction to fail so any execution reports for other
lanes will be blocked.

For DON execution transactions the behavior is now to skip the report
and emit a `SkippedReportExecution`. For manual execution we still
revert, otherwise the transaction will silently fail for the users.

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>

* remove bootstrapP2PIds (#1388)

## Motivation

Bootstrap P2P IDs are no longer needed in the OCR config in CCIPConfig.
See #1348's description for more details.

## Solution

Remove bootstrap P2P IDs from the OCR config in CCIPConfig.

## Related PRs
smartcontractkit/chainlink-ccip#89

* Integrate RMNRemote and OffRamp (#1360)

Remove the requirement for self-transmitted RMN blessings

Allow the commit DON to include RMN blessings at commitment time
This PR has a dependency on changes to
[chainlink-ccip](smartcontractkit/chainlink-ccip#84)

---------

Co-authored-by: Makram <[email protected]>

* Add report vs message source chain selector check (#1413)

The goal of this PR is to add an extra check in the execute function to
enforce that the report source chain selector matches its messages
source chain selector. Extra gas cost is about 50 gas per message.

Revert when `report.sourceChainSelector !=
(message.header.sourceChainSelector`

* Enforce signature verification for commit plugin in `OffRamp` (#1416)

## Motivation

The goal of this PR is to implement a check to ensure that the OCR3
signature verification is enabled for the commit plugin on config. This
reduces misconfig risks and impact because previously an accidental
setting of `isSignatureVerificationEnabled=false` for the commit plugin
in the `OffRamp` would have required a redeploy to be fixed.

## Solution

Add a check enforcing `isSignatureVerificationEnabled=false` for the
commit plugin in `_afterOC3Config`.

* Revert "reorder fields in MerkleRoot struct" (#1417)

## Motivation
Avoid forcing breaking changes on RMN nodes

## Solution
This reverts commit da570c5.

* Implement getTotalChainConfigurations (#1419)

## Motivation
The goal of this PR is to add a getter function to the `CCIPConfig`
contract to fetch the total number of chains configured. This value is
necessary to correctly call `getAllChainConfigs` and previously, the
only way to access it was through events.

## Solution
Implement `getTotalChainConfigurations`.

* Add sanity check in applyAllowListUpdates (#1420)

## Motivation
Currently, in the event of a misconfig where a destination chain's
`AllowListConfigArgs` `allowListEnabled` is set to false while providing
addresses to add to the allow list, the config will silently be skipped.

## Solution
Add a sanity check to detect `!allowListConfigArgs.allowListEnabled &&
`allowListConfigArgs.addedAllowlistedSenders.length > 0`

* Miscellaneous fixes and cleanups (#1421)

## Motivation

The goal of this PR is to fix several miscellaneous items.

## Solution

- Replaces some magic numbers with constants
- Default to `private` visibility for state variables that are not used
in test helpers
- Standardizes some natspec tags (consistent use of `@dev` instead of
`@notice` for state variables)
- Add a `ConfigSet` event in `CCIPConfig`
- Asserts full equality of `OCRConfigWithMeta[]` in `CCIPConfigTests`
- Add clarifying comments in `NonceManager` `applyPreviousRampsUpdates`

* CCIP-2815 commit report event updates (#1407)

## Motivation

OffRamp contract emits commitReport event as a Struct
to make the event Log filtering and searching more optimized, the report
properties are emitted as a event fields rather than emitting the struct

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>

* GasLimitOverride for manuallyExecute (#1375)

## Motivation
1. OffRamp should now use destExecData to extract the gas used for 
2. Manual execution should enable user to override the gas amount for
releaseOrMint per token.

## Solution
1. Removed int32 `maxTokenTransferGas` & `maxPoolReleaseOrMintGas` and
used the gas encoded in `RampTokenAmount.destExecData`
2. add a new Struct which holds the receiverExecutionGasLimit and
transferGasAmounts
```
struct GasLimitOverride {
    uint256 receiverExecutionGasLimit;
    uint256[] tokenGasOverrides;
}
```
tokenGasOverrides is an array of GasLimits to be used during the
relaseOrMint call for the specific tokenPool associated with token

Note: 
The gas limit can not be lowered as that could cause the message to
fail. If manual execution is done from an UNTOUCHED state and we would
allow lower gas limit, anyone could grief by executing the message with
lower gas limit than the DON would have used. This results in the
message being marked FAILURE and the DON would not attempt it with the
correct gas limit.

for the above we have kept a check that `tokenGasOverrides` < gas
encoded in `RampTokenAmount.destExecData`

---------

Signed-off-by: 0xsuryansh <[email protected]>
Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>

* Change data feeds from default to fallback in `FeeQuoter` (#1430)

## Motivation
Due to the gas cost of reading from data feeds, it is unlikely to be the
default option, we can therefore optimize the `getTokenPrice()` function
by defaulting to state instead of data feeds.

## Solution
Default to state reported prices and only fall back on data feed
external call if the reported prices are stale.

* Zp/update mockrouter (#1427)

Specifically, allow for empty message with zero gas.

## Motivation
EVM2EVMOffRamp checks the message for content and gaslimit and skips
calling ccip receive if they're not present.
Consequently the mock errors (ReceiverError) when sending only tokens to
a smart contract. This update will help chainlink-local correctly
process fork-based tests too.

## Solution
The MockRouter implementation of _routeMessage was last updated in
smartcontractkit/ccip#669 and is out of sync
with the logic in EVM2EVMOffRamp.

* Hybrid Token Pools: Remove problematic liquidity functions and add incoming supply lock (#1396)

## Motivation

Minor Changes to the HybridLockReleaseTokenPool to better meet Circle's
bridge requirements

1. Add additional supply lock on incoming messages when a proposal has
been created. I.E When a proposal for a chain selector is active, new
tokens cannot be released or locked to ensure better security control on
the destination-chain's token supply.
2. `withdrawLiquidity()` and `transferLiqudity()` have been removed to
avoid issues with the supply lock. Allowing for withdrawing liquidity
that has been provided would allow for the supply on the destination
chain to be greater than the locked amount on the source chain,
preventing a successful migration from occuring.

3. Comment fixes for invariants

* Make onRamp mutable in OffRamp (#1422)

## Motivation
The goal of this PR is to reduce the misconfig risk on the `OffRamp`
with the `onRamp` address. Currently, if we accidentally configure the
wrong `onRamp` address on an existing `OffRamp`, the fix would involve a
full redeployment + reconfig.

## Solution

- Make the `onRamp` mutable in the `OffRamp`.
- Add `onRamp` address verification in the `commit()` function
- Only allow `onRamp` address modification when `minSeqNr == 1`

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>

* update offramp layout (#1434)

## Motivation
Some header are outdated. 

The exec logic is above the commit logic, but then after the commit
logic we have the token handling logic, which is only used for exec. It
has been moved to be above the commit logic

* add CCIPEncodingUtils contract and gethwrapper (#1429)

## Motivation
I encountered a scenario w/ RMN where offchain needs to abi.encode a
[struct](https://github.com/smartcontractkit/ccip/blob/fc50683640bcfd73f0074d791fbaf7cdb6b29c71/contracts/src/v0.8/ccip/rmn/RMNRemote.sol#L90:L97)
that is not included in any public contract functions (as input param or
return value) and so it is not a part of the ABI or the go wrapper.
Offchain is currently doing a manual encoding by re-defining the type,
which I usually try to avoid at all costs. In the past, we've used a
Utils contract ([ex
here](https://github.com/smartcontractkit/chainlink/blob/develop/contracts/src/v0.8/automation/AutomationCompatibleUtils.sol)
from automation) to expose all private structs in public functions so
that offchain components can use E2E type safety when encoding /
decoding.

## Solution
Create a `CCIPEncodingUtils` contract and accompanying gethwrapper

* Remove TODO and rename messageValidator to messageInterceptor (#1438)

- Replaces `CCIPConfigTypes.ConfigState` `/// TODO: explain rollbacks?`
by rollbacks comment
- Renames `messageValidator` to `messageInterceptor` in ramps

---------

Co-authored-by: Rens Rooimans <[email protected]>

* update diagram (#1439)

* Make forwardFromRouter non reentrant (#1437)

## Motivation
Currently the `OnRamp` performs 3 separate calls to the `FeeQuoter` on
the hot path, including 2 in the `forwardFromRouter` function:

1. `getValidatedFee`
2. `processMessageArgs`
3. `processPoolReturnData`

This is done to follow the CEI pattern and guard against reentrancy due
to the untrusted calls to the pools. This is not optimal for gas but it
also makes contract interactions more complex.

## Solution

Implement a non reentrant flag and reorder the `forwardFromRouter`
function layout. The calls to the pools are now performed first, relying
on the reentrancy protection. We reduce the `FeeQuoter` calls in
`forwardFromRouter` to a single `processMessageArgs` call.

## Notes

Optimizer runs had to be decreased due to contract size which is why gas
diffs are showing a gas increase. At equivalent runs diffs are:
```
test_ForwardFromRouterExtraArgsV2_Success() (gas: -65 (-0.045%))
test_ForwardFromRouterSuccessLegacyExtraArgs() (gas: -65 (-0.045%))
test_ForwardFromRouterSuccessCustomExtraArgs() (gas: -65 (-0.045%))
test_ForwardFromRouter_Success() (gas: -65 (-0.045%))
test_ForwardFromRouterSuccessEmptyExtraArgs() (gas: -66 (-0.046%))
test_ForwardFromRouterExtraArgsV2AllowOutOfOrderTrue_Success() (gas: -65 (-0.057%))
test_forwardFromRouter_WithValidation_Success() (gas: 318 (0.113%))
test_E2E_3MessagesMMultiOffRampSuccess_gas() (gas: -1139 (-0.074%))
```

* Standardize message hashing functions (#1424)

## Motivation
The goal of this PR is to standardize the message hashing functions.
Currently we have equivalent but inconsistent message hashing functions:
```solidity
function _hash(Any2EVMRampMessage memory original, bytes memory onRamp) internal pure returns (bytes32)
```
and 
```solidity
function _hash(EVM2AnyRampMessage memory original, bytes32 metadataHash) internal pure returns (bytes32)
```

## Solution
Standardize to `function _hash(message, metadataHash)` and consistently
encode dynamic fields.

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>

* Validate transmitter lengths against F values (#1432)

## Motivation
Improves validations for OCR3 and CCIPConfig contracts

## Solution
Covers validations:
* `fChain <= F`, since `F` represents the full role DON
* Allows setting empty transmitters in CCIPConfig to allow
`len(transmitters) <= len(signers)`, while still enforcing
`len(transmitters) >= 3 * fChain + 1`
* Validates non-zero transmitters length in MultiOCR3
* Validates `len(signers) >= len(transmitters)` in MultiOCR3

### Tooling implications
* When setting OCR configs on OffRamp contracts, the list from
CCIPConfig must be filtered to exclude 0-bytes addresses. The reason for
this is that the MultiOCR3Base disallows zero addresses and duplicates
* Signer and transmitter duplication must be checked in tooling scripts
off-chain prior to setting CCIPConfig (otherwise it will result in
OffRamp breakage)
* `chainConfigs` must be set before OCR3 configs due to the added
`fChain == F` validation
* If `fChain` becomes higher than `F` after updates, existing OCR3
configs get invalidated without an on-chain check (since it is difficult
to do). There should be an off-chain validation script that prevents
this (first, F should be increased for all configs, only after which
fChain can be increased)
* `OffRamp` transmitters.length should be validated such that it meets
the `3 * fChain + 1` criteria off-chain, since `fChain` is not tracked
in the `OffRamp`


### Gas
| Function Name | min | avg | median | max | # calls |

|---------------------------------------------------------------------------|-----------------|--------|--------|---------|---------|
| validateConfig (without loop check) | 7040 | 20701 | 8436 | 122591 |
11 |
| validateConfig (with loop check) | 7040 | 21006 | 9132 | 123442 | 11 |

* Split RampTokenAmount into EVM2AnyTokenTransfer and Any2EVMTokenTransfer (#1435)

* Productionize RMN remote contract (#1431)

[Original PR](smartcontractkit/ccip#1308) -
there is some discussion here that is possibly still relevant)
[Open PR to add
cursing](smartcontractkit/ccip#1400) - I copied
the relevant parts for the `RMNRemote`.

Note: we are intentionally separating `RMNRemote` and `RMNHome` since
they have different timelines.

Productionize the RMNRemote and include it in integration tests

---------

Co-authored-by: Makram <[email protected]>

* Add RMN_V1_6_ANY2EVM_REPORT to RMNRemote and make public (#1447)

## Motivation

smartcontractkit/ccip#1431 (comment)

## Solution
Expose the constant as public

* merge FeePaid event into CCIPMessageSent (#1456)

## Motivation
Not sure actually why this was needed. Request was [made
here](https://smartcontract-it.atlassian.net/browse/CCIP-3446?atlOrigin=eyJpIjoiMTQ4ODMwNzU2N2I3NDI0NGE0ZDhiOTIyZTYwOTNjN2IiLCJwIjoiaiJ9).

This is the 2nd version, that alters the MessageSent struct. First
version [here](smartcontractkit/ccip#1452).

## Solution
Merge fields from FeePaid event into CCIPMessageSent event

* Add RMNHomeAddress in the OCR3Config (#1449)

Adding `rmnHomeAddress` in the `OCR3Config` struct + generated wrappers.

* Update pragma to most restrictive dependency or feature (#1466)

## Motivation
We have several contracts using `^0.8.0` but also custom errors, or a
child contract with custom errors. We also have several contracts at
`^0.8.0` but a dependency locked at `0.8.24`.

## Solution
Update all pragmas to the pragma of the most restrictive dep or the
minimum to enable custom errors (`0.8.4`)

* RMNHome (#1469)

New PR to have this be standalone

* ccip home (#1473)

Fresh PR

* fix snapshots

* Update gethwrappers

* [WIP] fix basic offchain logic (#14613)

* fix basic offchain logic

* type fixes

* bump chainlink-common, general fixes

* fix message hashing

* small fixes

* Update chainlink-common

* Fix commit-codec test.

* Fix error messages.

* Fix ccip reader tests (#14619)

* Fix exec codec.

* Launcher integration tests (#14648)

* CCIPHome basic usage framework

* Fix homeChain usage

* Fix calls in launcher test

* Add encoded configs

* Add more asserts

* Calling right function 😢

* new assertions and logging error

* Add don only once and update for all next operations

* Fix typo

---------

Co-authored-by: Makram Kamaleddine <[email protected]>

* core/capabilities/ccip: fix usdc reader test (#14652)

* fix lint

* Rename bluegreen to deployment and use active candidate naming (#14662)

* Fix smoke tests (#14655)

* Initial commit - fix AddDon in deployment tests

* Fix types and add logs

* more explicit config setting

* Rename bluegreen to deployment and use active candidate naming (#14662)

* contracts/ccip/capability: per (donId, pluginType) active config indexes (#1488)

* Add CCIPHome fixes

* Commit Encoding with new structs

* Update gethwrappers

* Commit Encode/Decode with new structure

* fix encoding, bump cl-ccip

* Update gethwrappers

* logging updates

* add rmn home and remote configuration

* fixes

* working int test, still some failing tests

* bump chainlink-ccip

* some more fixes

* lint, build

* more lint

---------

Co-authored-by: Makram Kamaleddine <[email protected]>
Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>

---------

Co-authored-by: Makram Kamaleddine <[email protected]>
Co-authored-by: Will Winder <[email protected]>
Co-authored-by: Abdelrahman Soliman (Boda) <[email protected]>
Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>

* tidy

* lint again

* remove stale files

* bump chainlink-ccip again

* bump with cl-ccip from main

* fix build

* fix

---------

Signed-off-by: 0xsuryansh <[email protected]>
Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
Co-authored-by: Suryansh <[email protected]>
Co-authored-by: Ryan <[email protected]>
Co-authored-by: Makram <[email protected]>
Co-authored-by: Ryan Hall <[email protected]>
Co-authored-by: kanth <[email protected]>
Co-authored-by: Zubin <[email protected]>
Co-authored-by: Josh Weintraub <[email protected]>
Co-authored-by: Evaldas Latoškinas <[email protected]>
Co-authored-by: nogo <[email protected]>
Co-authored-by: Will Winder <[email protected]>
Co-authored-by: Abdelrahman Soliman (Boda) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants